Code & Security Review

Solid, well-scoped refactor: collapsing three near-identical findBinary() impls into one default on CodingAgent (override → PATH walk → fallback) is the right shape, the kind raw-values line up with the JSON keys, the stale-override fall-through is handled, and the config field decodes forward-compatibly. Verified launchCommandToken (codex/agent/claude) and kind (codex/cursor/claude-code) both resolve correctly, and CrowCore builds clean. One blocking test defect and one behavioral concern.

Critical Issues

🔴 Red — BinaryOverrides test suite is racy and fails reproducibly under parallel execution
Packages/CrowCore/Tests/CrowCoreTests/BinaryOverridesTests.swift

All four tests mutate the process-global BinaryOverrides.shared singleton, and Swift Testing runs tests in parallel within a suite by default. The per-test defer { set([:]) } resets don't isolate anything — a concurrent test's set(...) clobbers another's state mid-assertion. The PR's "all green" claim doesn't hold: running swift test --filter BinaryOverrides 6× here failed 5 times, e.g.:

✘ emptyByDefault() — (path(for: .cursor) → "/tmp/agent") == nil
✘ setStoresRawKeyedByAgentKind() — (path(for: .cursor) → nil) == "/tmp/agent"
✘ setReplacesPreviousMap() — (path(for: .cursor) → nil) == "/tmp/agent"

This will flake CI. Fix: serialize the suite — @Suite("BinaryOverrides", .serialized) — and likewise add .serialized to @Suite("OpenAICodexAgent") in Packages/CrowCodex/Tests/CrowCodexTests/OpenAICodexAgentTests.swift, since its two new findBinary* tests mutate the same global (that suite passed 6/6 here only by timing luck — findBinaryHonorsBinaryOverride vs findBinaryIgnoresOverrideWhenPathMissing race on codex). .serialized is sufficient because only these two suites touch the global, and they live in separate targets.

Code Quality

🟡 Yellow — PATH walk for Cursor's generic agent binary name risks false-positive registration
Packages/CrowCore/Sources/CrowCore/Agent/CodingAgent.swift:129 + CursorAgent.launchCommandToken = "agent"

The old impl checked only three explicit .../agent paths. The new PATH walk registers Cursor for any executable named agent anywhere on the user's $PATH. agent is a notably generic name — CI build agents (Azure DevOps, TeamCity), etc., ship a binary literally called agent. On such a machine Crow would now mis-register Cursor and later launch the wrong binary. codex/claude are specific enough not to collide; agent is the outlier. Worth at minimum a code comment acknowledging the tradeoff, and ideally a sanity check (or documenting defaults.binaries.cursor as the escape hatch in the user-facing config docs).

Security Review

Strengths:

• No injection surface added — findExecutable does filesystem isExecutableFile checks, not shell evaluation, and POSIX empty-PATH entries (which would mean CWD) are dropped by split(separator:), avoiding accidental CWD execution.
• BinaryOverrides is an opt-in absolute-path override from the user's own config; stale/missing paths fall through safely rather than breaking registration.

Concerns: None beyond the Yellow above.

Summary Table

Recommendation: Request Changes — driven by [1 Red, 1 Yellow, 0 Green] findings.

🐦‍⬛ Reviewed by Crow via Claude Code

Code & Security Review

Refactors per-agent binary discovery into a single default CodingAgent.findBinary() impl: explicit defaults.binaries.<kind> override → PATH walk (ShellEnvironment.findExecutable) → hardcoded fallbackCandidates. This correctly fixes the CROW-484 gap where npm-global / nvm / Volta / pnpm / asdf installs were invisible. Verified by building and running the full test suites for CrowCore, CrowCodex, CrowCursor, and CrowClaude — all green (269 + 29 + 29 + 41 tests).

Critical Issues

None.

Security Review

Strengths:

• No new attack surface: discovery is read-only (isExecutableFile), no shell-out, no command construction from user input.
• Stale-override handling is correct and defensive — findBinary() re-verifies isExecutableFile(atPath:) on the configured path before returning it (CodingAgent.swift:124), so a moved/uninstalled binary falls through to PATH/fallback rather than pinning a dead path or breaking registration. Covered by findBinaryIgnoresOverrideWhenPathMissing.
• BinaryOverrides singleton is properly guarded by NSLock; test suites that mutate the shared state are .serialized to avoid cross-suite leakage.

Concerns:

• None blocking. The Cursor agent PATH-collision risk (a CI runner's agent binary could resolve as Cursor's CLI on a build machine) is real but is thoroughly documented at CursorAgent.swift and fully mitigated by the defaults.binaries.cursor escape hatch. Acceptable tradeoff for a desktop workstation app.

Code Quality

• Clean abstraction: the discovery logic now lives in one place and each agent just declares fallbackCandidates; hasCommand is correctly re-expressed as findExecutable(_:) != nil so the two stay in sync (pinned by hasCommandAgreesWithFindExecutable).
• ConfigDefaults.binaries decoding is forward/backward-compatible — a missing key decodes to [:] (configDefaultsBinariesDefaultsEmpty), and the field round-trips through the synthesized encoder.
• AppDelegate.launchMainApp config-load reorder is safe: config is loaded once up front, BinaryOverrides.shared is populated before the registration gates run, and the later duplicate load was correctly removed. The new registered at <path> NSLog aids field debugging.
• Good test coverage across all three layers (override map, PATH resolver, config decode, agent-level honor/ignore).

Summary Table

Recommendation: Approve — driven by [0 Red, 0 Yellow, 1 Green] findings. The single Green item (Cursor agent name collision) is already documented and mitigated in the PR.

🐦‍⬛ Reviewed by Crow via Claude Code